Add support for otlp trace exporters #5477
Conversation
Sovietaced
force-pushed
the
otlp
branch
3 times, most recently
from
0d2f022
to
37b8412
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5477 +/- ##
==========================================
- Coverage 61.01% 60.99% -0.03%
==========================================
Files 794 793 -1
Lines 51441 51335 -106
==========================================
- Hits 31388 31310 -78
+ Misses 17161 17139 -22
+ Partials 2892 2886 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -33,7 +35,7 @@ var ( | |||
| ) | |||
|
|
|||
| type Config struct { | |||
| ExporterType ExporterType `json:"type" pflag:",Sets the type of exporter to configure [noop/file/jaeger]."` | |||
| ExporterType ExporterType `json:"type" pflag:",Sets the type of exporter to configure [noop/file/jaeger/otlpGrpc/otlpHttp]."` | |||
| FileConfig FileConfig `json:"file" pflag:",Configuration for exporting telemetry traces to a file"` | |||
| JaegerConfig JaegerConfig `json:"jaeger" pflag:",Configuration for exporting telemetry traces to a jaeger"` | |||
There was a problem hiding this comment.
All the OTEL exporter configuration can come from environment variables so I didn't really see any value in 1) deviating from OTEL and 2) trying to copy all of the settings that can be configured via the OTEL environment variables.
There was a problem hiding this comment.
Good callout @Sovietaced. My hesitation is that it deviates with other trace configuration and how flyte components are configured generally. It adds ambiguity to a component's behavior and how to configure it. I'm inclined to keep consistent.
If it's helpful I have a private version of this I've been playing with to use OTEL + expose some sampling config. I could pull that in instead and save you some cycles. Wdyt?
There was a problem hiding this comment.
I can see the user experience being a little nicer for some folks if more configuration came through the YAML dictionaries in the helm chart but I'm personally not interested in trying to duplicate the OTEL standardized environment variables so you're probably better of pulling in changes from your fork.
There was a problem hiding this comment.
Sounds good. I have a draft up at #5504. Let me pull in a few more of the changes you proposed here as well
|
@hamersaw @andrewwdye can you plz review this PR? |
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
|
Closing in favor of: #5504 |
Tracking issue
Closes #5476
Why are the changes needed?
Distributed traces can only be exported using Jaeger with the current code. The jaeger protocol has been deprecated in favor of the open telemetry protocol (OTLP). Additionally, if we're using OTEL client libraries Flyte should also just support their native protocol as well.
We should also deprecate use of the Jaeger exporter and slate it for removal.
What changes were proposed in this pull request?
This pull request adds two new trace exporter strategies:
otlpGrpc(OTLP over gRPC) andotlpHttp(OTLP over HTTP).How was this patch tested?
This patch was tested locally in our non production environment.
Check all the applicable boxes
Related PRs
Docs link